Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: Update keytar and whats-my-line (allows building on Node 18 or newer) #35

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Sep 28, 2023

This dependency bump is motivated by seeking to get rid of outdated nan, which was an indirect dependency via keytar. Dropping nan should hopefully allow compiling native C/C++ code against newer and newer versions of NodeJS and Electron -- "hopefully".

This version of keytar drops its dependency on nan, instead using "N-API", now officially renamed to "Node-API". Hopefully this means it will not need changes to maintain forward-compatibility with future versions of NodeJS and Electron -- "hopefully."

So, there may also be benefits for upgrading to newer versions of Electron -- I haven't tested this, I just want to be able to bootstrap Pulsar core repo on newer system versions of NodeJS, however irrelevant system NodeJS version may be to final execution in Electron.

...

This is the latest, and final upstream release of atom/node-keytar; The upstream repository is now archived.

(See: https://github.com/atom/node-keytar/releases)

If we want to see changes in keytar beyond this, we must search for a suitable, existing fork, or create one ourselves.

Verification process:

  • Can build this package with NodeJS 18 and 20 on the system now, not just NodeJS 16 or lower.
  • Can still successfully store a GitHub API token with github package after this change.

This dependency bump is motivated by seeking to get rid of outdated
nan, which was an indirect dependency via keytar. Dropping nan should
hopefully allow compiling native C/C++ code against newer and newer
versions of NodeJS and Electron -- "hopefully".

This version of keytar drops its dependency on nan, instead using
"N-API", now officially renamed to "Node-API". Hopefully this means
it will not need changes to maintain forward-compatibility with future
versions of NodeJS and Electron -- "hopefully."

So, there may also be benefits for upgrading to newer versions of
Electron -- I haven't tested this, I just want to be able to bootstrap
Pulsar core repo on newer system versions of NodeJS, however
irrelevant system NodeJS version may be to final execution in
Electron.

...

This is the latest, and final upstream release of atom/node-keytar;
The upstream repository is now archived.

If we want to see changes in keytar beyond this, we must search for a
suitable, existing fork, or create one ourselves.
This is for Python 3.12+ compatibility in older node-gyp
Switches superstring indirect dependency to Pulsar's fork as well
Uses lockfile v1, which ppm (npm 6) is more compatible with
Also update to a commit of whats-my-line where superstring
is specified as a tarball URL, not a git URL.

Otherwise, no changes.

Just using a simpler format (tarballs), which are quicker
to fetch and quicker to install than git URL dependencies,
as well as being just a lot simpler to deal with under the hood.
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Nov 11, 2023

This PR's goals were achieved. The module builds in CI on Node 18 on all three major OSes... even if CI isn't passing in general in this repo (before/regardless-of this specific PR's changes).

The point is: The project builds on Node 18. Also build on macOS with Node 20 on my machine. Of course, it is working in Pulsar as well, so it works with/builds against Electron 12. (Tested with ppm link).

This is sort of ready, but it would be slightly cleaner to wait for github.com/pulsar-edit/whats-my-line/pull/6 to be done and use the merged result of that instead of the commit from the as-yet-unmerged PR branch over there... So I'm marking this as draft until github.com/pulsar-edit/whats-my-line/pull/6 is done, but there's nothing substantive that still needs to be done here. EDIT: Done.

Hopefully can help unblock one more thing with regards to Electron upgrades in Pulsar core repo.

@DeeDeeG DeeDeeG marked this pull request as draft November 11, 2023 00:08
@DeeDeeG DeeDeeG changed the title deps: Update keytar to 7.9.0 (drops nan --> N-API) deps: Update keytar and whats-my-line (allows building on Node 18 or newer) Nov 11, 2023
This is just the commit from *after merging* the whats-my-line PR
that specified superstring as a tarball URL rather than as a git URL.

No code changed, this just points to the merge commit after merging,
instead of the PR branch tip pre-merge.

(For the benefit of folks who spelunk through the commit histories.
This is a slightly tidier landmark in the whats-my-line repo
to point to: a merge commit into `master` branch,
rather than a random-looking pre-merge commit. No change otherwise.)
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Nov 11, 2023

Okay, I updated to the commit of whats-my-line repo from just after pulsar-edit/whats-my-line#6 was merged.

So, this is really ready now. Marking "Ready for review" (taking out of draft status).

@DeeDeeG DeeDeeG marked this pull request as ready for review November 11, 2023 02:04
Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gonna approve this one.
While I can't verify things work (As the CI is failing us) if you can build it locally that's fine by me lol, and sounds like a plan then to get this one merged, and get these updates into the core.

I know this one has been open forever, so I'm happy you've stuck by it's side

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants